Skip to content

SQC-871 Implements socket serialization for transferring socket over RPC#6863

Open
Ltadrian wants to merge 2 commits into
cloudflare:mainfrom
Ltadrian:dlapid/jsrpc_socket
Open

SQC-871 Implements socket serialization for transferring socket over RPC#6863
Ltadrian wants to merge 2 commits into
cloudflare:mainfrom
Ltadrian:dlapid/jsrpc_socket

Conversation

@Ltadrian

@Ltadrian Ltadrian commented Jul 6, 2026

Copy link
Copy Markdown
  • Adds TransferredSocketStream class that handles implementing underlying AsyncIoStream IO operations for deserializing socket
  • Socket deserialization unwraps raw kj streams and builds non-deferred stream to pass into TransferredSocketStream
  • Adds js-rpc-socket-test tests
  • Add outbound interceptor test to showcase full end to end use case of loopback binding

RPC
- Adds TransferredSocketStream class that handles implementing
  underlying AsyncIoStream IO operations
- Socket serialization/deserialzation unwraps raw kj streams and builds
  non-deferred sockets
- Fix and assert existing test that returns socket over RPC and can read
  bytes
- Add outbound interceptor test to showcase returning socket over RPC
@Ltadrian Ltadrian changed the title Implements socket serialization for transferring socket over RPC SQC-871 Implements socket serialization for transferring socket over RPC Jul 6, 2026
// Serialize the readable and writable streams as separate externals
readable->serialize(js, serializer);

writable->serialize(js, serializer);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be aware the the JsReadableStream and JsWritableStream work necessarily will impact this. JsReadableStream has already landed. readable is no longer a jsg::Ref<ReadableStream> but is wrapped by a JsReadableStream. The serialization story is still a work in progress.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you I wasn't aware of JsReadableStream before working on this. I've caught up on the implementation in the internal repo workerd and it looks like it hopefully be too much of a lift to refactor some of these parts. I'll keep my eye out for when this lands in public repo 😊 Do you think it would be a blocker to support both JsReadableStream/JsWritableStream before landing this socket serialization?

// JsRpcCustomEvent is canceled), so their pumpTo() must not be deferred past the context's
// lifetime.
sysStreams.readable = newNoDeferredProxyReadableStream(kj::mv(sysStreams.readable), ioContext);
auto readable = js.alloc<ReadableStream>(ioContext, kj::mv(sysStreams.readable));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With JsReadableStream already landed, this would become JsReadableStream::create(...)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(JsReadableStream is not yet in the public repo... should arrive there soon)

// No opened-gate on the writable: the connection is already established (opened resolves below),
// and passing a gate is unnecessary here.
auto writable = js.alloc<WritableStream>(ioContext, kj::mv(sysStreams.writable),
ioContext.getMetrics().tryCreateWritableByteStreamObserver());

@jasnell jasnell Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would change once JsWritableStream lands

Comment thread src/workerd/api/streams/readable.h
@jasnell

jasnell commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

A few questions...

  1. How does socket upgrade (startTLS) work with a serialized socket?
  2. It looks like isDefaultFetchPort is not set?
  3. There doesn't seem to be any handling of opened state. A socket uses the openedPromise to signal when the socket is actually opened/established. From the looks of it here, it would be possible to serialize a still not-yet-open socket but the receiving side would appear to be opened? It's not clear.
  4. What if the socket is already being closed? Or is already closed?

@dom96 dom96 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, like James mentioned, I'd repeat that currently as things stand there is a need for more synchronisation with the various socket states (closed/open). But perhaps that's planned since this is still a draft :)

In general though I think this is a good start. I left a few comments about various things I'd improve as well, but nothing major.

Comment thread src/workerd/api/tests/js-rpc-test.js
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++
Comment thread src/workerd/api/sockets.c++
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/api/sockets.c++ Outdated
Comment thread src/workerd/io/worker-interface.capnp
@Ltadrian

Ltadrian commented Jul 9, 2026

Copy link
Copy Markdown
Author

A few questions...

  1. How does socket upgrade (startTLS) work with a serialized socket?
  2. It looks like isDefaultFetchPort is not set?
  3. There doesn't seem to be any handling of opened state. A socket uses the openedPromise to signal when the socket is actually opened/established. From the looks of it here, it would be possible to serialize a still not-yet-open socket but the receiving side would appear to be opened? It's not clear.
  4. What if the socket is already being closed? Or is already closed?

@jasnell

  1. In the case of Hyperdrive needs, socket upgrade isn't needed as Hyperdrive will handle TLS to the origin. In the generalized socket case I am not sure. I imagine that we would need to somehow allow the transferred socket to upgrade tls and start tls handshake but I would probably prefer if the socket has already been upgraded before transfer instead?
  2. Updated to add isDefaultFetchPort to worker-interface.capnp and serialize/deserialize ✅
  3. I thought about this for a while and I think throwing an error is fair if the socket is still pending/closed. I can't think of a good case where receiving a socket and needing to await on socket.opened would be a fair user experience. I would prefer that the socket is assumed to be opened and connected. I've gone ahead and implemented some checks on serialize to prevent serializing the socket when that state is pending/closed but fully open to other implementation ideas here
  4. Throwing error on serialize feels fair to me. There is still the case of where on serialize the socket is opened, but during transit the connection closes and we assume the serialize socket info is still opened on deserialize. This should naturally resolve to closed but might show some disconnect errors? Let me know if this behavior is not preferred

@Ltadrian

Ltadrian commented Jul 9, 2026

Copy link
Copy Markdown
Author

Yeah, like James mentioned, I'd repeat that currently as things stand there is a need for more synchronisation with the various socket states (closed/open). But perhaps that's planned since this is still a draft :)

In general though I think this is a good start. I left a few comments about various things I'd improve as well, but nothing major.

@dom96

Thank you for the initial review, I have spent some extra time addressing the feedback and have added a second commit to this merge request to handle the various states I initially put on TODO: halfOpen, opened, closed

I'll re-review my second commit today again but this seems to be functional now

(opened/closed/allowHalfOpen/secureTransport)

Carry the origin socket's runtime state over JS RPC and reconstruct
equivalent behavior on the receiving side.

- Wire format: add secureTransport (new enum), isDefaultFetchPort,
  localAddress,
  and allowHalfOpen to External.socket; fix the socket external doc
  comment.
- opened: track settled state (OpenedState enum); serialize() throws
  DataCloneError unless OPENED; deserialize() resolves it from the
  transferred
  SocketInfo and marks the socket OPENED.
- allowHalfOpen: carried across transfer; when false, re-establish the
  read-EOF -> write-side auto-close on the receiving side.
- closed: extract shared disconnect wiring (used by setupSocket) and
  resolve `closed` on disconnect for transferred sockets.
- deserialize scope: the EOF and closed wiring attach jsg `.then()`
  continuations (illegal under the no-JS deserialize scope), so defer
  them to microtasks; kj-side signals are latched synchronously so
  nothing is missed.
- Tests: allowHalfOpen honored across transfer, unopened transfer
  throws,
  `closed` resolves on close(); js-rpc-test awaits `opened` before
  returning.
@Ltadrian Ltadrian force-pushed the dlapid/jsrpc_socket branch from b7082f5 to a44dee4 Compare July 9, 2026 19:25
@Ltadrian Ltadrian marked this pull request as ready for review July 10, 2026 17:34
@Ltadrian Ltadrian requested review from a team as code owners July 10, 2026 17:34
@codspeed-hq

codspeed-hq Bot commented Jul 10, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 9.77%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
✅ 71 untouched benchmarks
⏩ 129 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
bodyWithHeaders[Response] 34.2 µs 31.2 µs +9.77%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing Ltadrian:dlapid/jsrpc_socket (a44dee4) with main (a77b7aa)

Open in CodSpeed

Footnotes

  1. 129 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants